[mlir][vector] Use llvm::Align when constructing vector load and stores.
#152207
Conversation
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-vector Author: Erick Ochoa Lopez (amd-eochoalo) ChangesThis patchset adds a small struct which represents the alignment in terms of bytes. Adding an explicit struct instead of using integers allows for easier reading and avoids confusion since the alignment can be given in different units. See for example the following RFC. Full diff: https://github.com/llvm/llvm-project/pull/152207.diff 2 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.h b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.h
index 364c1728715e8..b5ae0123d8c53 100644
--- a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.h
+++ b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.h
@@ -77,6 +77,15 @@ struct VectorDim {
int64_t dim;
bool isScalable;
};
+
+struct AlignmentBytes {
+ uint64_t alignment = 0;
+ AlignmentBytes() = default;
+ explicit AlignmentBytes(uint64_t alignment) : alignment(alignment) {};
+ explicit operator bool() const { return alignment; }
+ uint64_t getAlignment() const { return alignment; }
+};
+
BroadcastableToResult
isBroadcastableTo(Type srcType, VectorType dstVectorType,
std::pair<VectorDim, VectorDim> *mismatchingDims = nullptr);
diff --git a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
index 3885439e11f89..3ebb5721d7181 100644
--- a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
+++ b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
@@ -1729,18 +1729,18 @@ def Vector_LoadOp : Vector_Op<"load", [
"Value":$base,
"ValueRange":$indices,
CArg<"bool", "false">:$nontemporal,
- CArg<"uint64_t", "0">:$alignment), [{
+ CArg<"AlignmentBytes", "AlignmentBytes()">: $alignment), [{
return build($_builder, $_state, resultType, base, indices, nontemporal,
- alignment != 0 ? $_builder.getI64IntegerAttr(alignment) :
+ alignment ? $_builder.getI64IntegerAttr(alignment.getAlignment()) :
nullptr);
}]>,
OpBuilder<(ins "TypeRange":$resultTypes,
"Value":$base,
"ValueRange":$indices,
CArg<"bool", "false">:$nontemporal,
- CArg<"uint64_t", "0">:$alignment), [{
+ CArg<"AlignmentBytes", "AlignmentBytes()">: $alignment), [{
return build($_builder, $_state, resultTypes, base, indices, nontemporal,
- alignment != 0 ? $_builder.getI64IntegerAttr(alignment) :
+ alignment ? $_builder.getI64IntegerAttr(alignment.getAlignment()) :
nullptr);
}]>
];
@@ -1847,9 +1847,9 @@ def Vector_StoreOp : Vector_Op<"store", [
"Value":$base,
"ValueRange":$indices,
CArg<"bool", "false">:$nontemporal,
- CArg<"uint64_t", "0">:$alignment), [{
+ CArg<"AlignmentBytes", "AlignmentBytes()">:$alignment), [{
return build($_builder, $_state, valueToStore, base, indices, nontemporal,
- alignment != 0 ? $_builder.getI64IntegerAttr(alignment) :
+ alignment ? $_builder.getI64IntegerAttr(alignment.getAlignment()) :
nullptr);
}]>
];
|
kuhar
requested review from
krzysz00,
banach-space,
fabianmcg,
dcaballe,
tyb0807 and
Groverkss
There was a problem hiding this comment.
Thank you for starting the conversation on this!
Since we haven't had the discussion on bit alignments on the RFC, and LLVM already has llvm::Align and uses it for the load instruction, I think it's better to keep status quo or use llvm::Align instead of introducing a new struct.
|
@fabianmcg thanks! I didn't know about this struct. I've reverted the changes I had and used llvm::Align instead. |
amd-eochoalo
changed the title
llvm::Align when constructing vector load and stores.
|
Please update the description of the commit before merging. |
I think you ultimately mean the PR description that gets used for the commit message after squashing? |
Yes. |
This patchset uses
llvm::Alignwhen constructingvector.{load,store}operations. The use ofllvm::Alignallows us to specify the unit of alignment and strongly type alignment as opposed to having just unsigned integers.